Repository navigation
Conversation
4151e84 to
d7fbb9e
Compare
d7fbb9e to
62d90dc
Compare
|
I have updated the text to better clarify the exact comparisons for each of the supported attributes, |
62d90dc to
ff338e3
Compare
|
Dear TC, If so kindly let me know how to proceed. |
JelleZijlstra
left a comment
There was a problem hiding this comment.
I'm generally supportive but I think this change needs some more editing, and I'd like support in at least some type checkers (at least an open PR) before we start mandating this pattern.
c575c15 to
c951b00
Compare
|
Dear typing council, However now it appears that this PR is in a Catch-22 situation; Lacking response I plan close this in a few weeks |
This is not the case at all. It is very typical for type checkers to implement features even if they are not in the spec. It is very helpful to have feedback from type checker authors to be able to understand potential problems with spec changes. I think this has worked quite well in the past.and if a feature is useful enough there is usually a type checker that will implement it. |
carljm
left a comment
There was a problem hiding this comment.
Thanks for your patience, @Josverl. I'm not sure we've fully reached consensus yet on what should be supported here, but I also generally support making this clearly specified. I also think this still needs some editing to give a clear and unambiguous presentation of the requirements.
|
Thanks all for the renewed attention and comments.
|
b3b76c1 to
98e3697
Compare
|
Again thanks for the feedback and suggestions to improve the spec. I processed the comments and suggestion - and commented or marked them as resolved to keep track. Open topics. 1. 2-Tuple or 3-Tuple version comparisons.
2. Should 3. Scope I think the commits should be squashed when merging - I've kept them separate for now. |
|
We could eliminate open-question (2) and at least partially address (3) by just considering containment checks as out of scope to specify for now. Considering they don't add any new capability, just allow writing more concise code, they aren't really necessary. We could leave this out of the spec / conformance suite for now and up to individual type checkers. Similarly maybe for |
+1
Unlike containment checks, |
That would be disappointing, especially as one of the first responses in May'25 in DPO included :
But reality is that there different opinions based on different considerations, and
|
|
Not closed in anger - just a typo |
|
Gentle reminder for additional feedback. |
|
Here's a summary of what type checkers already support (courtesy of Codex):
So nobody supports any |
This is proposed for the spec in python/typing#2173. It is trivial to support in mypy and seems useful, so let's add it.
Inline list and set membership tests now preserve the values of their
individual elements, allowing ty to determine when a condition is always
true or false. With the target platform set to Linux, both `sys.platform
in ["linux", "windows"]` and `sys.platform in {"linux", "windows"}`
select only the matching branch, so an assignment there is inferred as
`Literal[1]`.
Reuse the precise element extraction used for iteration and membership
narrowing without changing the inferred container type. Lists and sets
use the existing tuple membership equality evaluator, including its
identity-or-equality behavior. For sets, assume equality is an
equivalence relation and equal objects have equal hashes. Checking
reflexive equality across union alternatives also improves existing
tuple membership inference.
Context is that in python/typing#2173 people are
wanting to specify that `sys.platform in ["linux", "windows"]` should
work. This makes it work in ty so that we can be fine with any outcome
of that specification change.
## Test plan
Mdtests cover platform-dependent branch reachability; positive and
negative membership in inline lists and sets; inferred literal values
and unions; fixed-length unpacking; stored mutable containers; custom
equality and identity; comparison chains; and preservation of enum-set
narrowing.
## Ecosystem report
The comparison for head `90111f10da9359722abb903351b89aaae0f9bcba` adds
four `redundant-condition-strict` warnings. The paasta and SciPy
warnings correctly identify disjoint membership checks. The dd-trace-py
warning exposes existing limitations in modeling dynamic `setattr`; the
Scrapy warning exposes existing attribute narrowing across method calls
and `await`. Equivalent equality checks already exhibit both limitations
on the base revision. These existing limitations are accepted for this
change. See the [full ecosystem
report](#28676 (comment))
for minimized examples and exact comparison details.
|
Just to avoid unneeded waiting: |
|
@carljm, CI also seems to have forgotten that I did sign the CLA with all my identities, re-signing is not possible either. |
carljm
left a comment
There was a problem hiding this comment.
I think the CLA signing failure is because your latest commit (unlike the previous ones) is authored by Jos_Verlinde@hotmail.com, which GitHub can't associate with your GitHub account. If you can amend that commit with a recognized email address and force push, I suspect the CLA signing check will pass.
Barring my two nits below, I think this PR is ready for conformance tests. @jorenham @davidhalter @JelleZijlstra @rchen152 any objections to the spec updates here, before @Josverl writes conformance tests?
|
I'll try to take a look this evening |
jorenham
left a comment
There was a problem hiding this comment.
Looks good overall, thanks. I left some comments for some minor issues / nits / suggestions.
| Required forms and optional extensions | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| Type checkers must support the forms specified above as required. Support for additional |
There was a problem hiding this comment.
consistency nit
| Type checkers must support the forms specified above as required. Support for additional | |
| Type checkers should support the forms specified above as required. Support for additional |
There was a problem hiding this comment.
I do not think that "should" can be combined with "required" in this sentence.
That would degrade the meaning of the "required" statements in the prior paragraphs, which I think is incorrect.
There was a problem hiding this comment.
IMO you should just drop the sentence. The spec is clear in the "above" cases where a type checker has to support it and where it's optional, so the sentence feels unnecessary.
bc8965e to
d2c4133
Compare
…atform checks. Signed-off-by: Jos Verlinde <jos_verlinde@hotmail.com>
d2c4133 to
e612515
Compare
|
@carljm, @jorenham Also have tried to make a start on the conformance tests in #2375. CLA signature appears to have been caused by some parts of the CLA check assuming that smtp e-mail addresses are case sensitive, but that was solved simply. |
|
@Josverl Why do we need to do conformance tests in a separate PR? I would find it much easier to review, approve, and merge the spec changes and conformance tests together, in the same PR. Conformance tests may reveal changes that we need to make to the spec, and we need to ensure they are consistent with each other. |
The original plan I was set out on was much simpler, and I explicitly started out with: "This is my first post to the Python discuss, so please be gentle." And was guided by the TC decision process that documents:
This is my first PR to "the big" Python, it is open more than 10 months with over 150 comments,
to be very clear, I could not have done this without all your help, but it has taken over 17 months since first discussion. ( with time-gaps both from all involved) On the conformance tests I replied earlier :
So what are the rules for creating conformance test , for functionality that is not currently supported ? Note that the TC process document does not mention conformance test.
To extend beyond that, is perhaps apparent you. To me it is not. Yes, I can and have set an AI to work, and revised the results to avoid the most apparent mistakes. What is the process after that ? So that is why I split them , because I would not want to loose this part, and currently I am unable to commit to finish something that I cannot fully understand the implications of. I would welcome a conversation on the challenges and opportunities involved in a more private setting. |
|
@Josverl are you on Discord? I'd be happy to connect there and talk through issues in a more informal way. (I can't speak for Carl but he's also on Discord and might also be willing to chat there.) |
Yes, thanks for the offer. I shared my details via E-mail. |
|
Yes, happy to discuss in Discord! To answer a few specific questions/comments:
|
davidhalter
left a comment
There was a problem hiding this comment.
First of all, thank you @Josverl for staying on this for such a long time. This is one of the most discussed issues and it's easy to give up at some point. Also thank you to Carl for doing most of the review work.
The PR is in a very good shape IMO. I have a few questions that are directed to other people than @Josverl, and a small suggestion to drop a sentence, otherwise no work should be necessary.
I would definitely not reduce the scope for now. While I would be grateful for conformance tests here I'd rather merge it as is and do the conformance tests later if @Josverl decides to not work on this anymore, since I think it's in a really good state and a massive improvement from the previous version. Though if you @Josverl still have some energy left in the tank, conformance tests would of course be awesome. :)
Thanks all!
| ^^^^^^^^^^^^^^ | ||
|
|
||
| Type checkers may optionally support comparisons of ``os.name`` with string | ||
| literals. |
There was a problem hiding this comment.
Why is this optional? Sorry I haven't followed the discussion for a while and I'm sure it has been discussed. It might make sense to make this mandatory as well.
There was a problem hiding this comment.
Because it's currently optional in the the conformance suite:
typing/conformance/tests/directives_version_platform.py
Lines 61 to 75 in d06c5dd
There was a problem hiding this comment.
Thanks! IMO we should probably make that mandatory then in a separate follow up PR.
| Required forms and optional extensions | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| Type checkers must support the forms specified above as required. Support for additional |
There was a problem hiding this comment.
IMO you should just drop the sentence. The spec is clear in the "above" cases where a type checker has to support it and where it's optional, so the sentence feels unnecessary.
| The configuration options should allow users to specify the target values for these | ||
| symbols, so that type checkers can evaluate the version and platform checks | ||
| correctly. The exact mechanism and names for these configuration options are | ||
| implementation-specific and defined by each type checker. |
There was a problem hiding this comment.
This is more of a question to other type checker authors: Is this already implemented somewhere? I definitely like that sys.implementation.name is added, that seems like a very useful addition especially in stubs. The implementation version is probably also needed, but that asks the question: How should I infer the implementation version? Should I just default to a nullable sys.implementation.version if the implementation cpython is not used and the version is not provided?
IMO it would be nice if we added this to the conformance tests as well, but that might be a little more complicated since we would need a way to specify the implementation/version for a specific test file. So I don't think this is necessary for this PR, but I would definitely appreciate if someone of us (looking at myself as well) would do the work here.
This PR expands and clarifies the specification for version, platform, and Python implementation checks:
sys.implementation.version.sys.platform.startswith().References :